feat(ingo): use FQCN Variable references and vendor-namespaced classes#34
Conversation
TDannhauer
left a comment
There was a problem hiding this comment.
Summary
Small, focused PR (7 files, +8/−8) that:
- Renames
Ingo\Form\V3→Horde\Ingo\Form\V3in the two custom Variable classes - Replaces
'ingo:Longemail'/'ingo:folders'with FQCN::classreferences in three legacy forms - Bumps
horde/formfrom^3to^3.1incomposer.jsonand.horde.yml
This aligns ingo with horde/form 3.1.0 FQCN support and with ingo's existing Composer PSR-4 map ("Horde\\Ingo\\": "src/").
What looks good
Namespace correction is necessary. The Variable classes lived under Ingo\Form\V3 while composer.json maps Horde\Ingo\ → src/. Resolution only worked because horde/form 3.1 tries Horde\Ingo\… first (PSR-4 loads the file), then falls back to Ingo\Form\V3\… after the file is already included. Moving to Horde\Ingo\Form\V3 fixes that properly.
Dependency bump is correct. FQCN type strings and vendor-namespaced app:type resolution are new in horde/form 3.1.0. ^3.1 || dev-FRAMEWORK_6_0 is the right constraint.
Call-site coverage is complete. All four custom-variable usages are updated (Forward, Vacation ×2, Spam). Inline \Horde\Ingo\Form\V3\…::class matches the style shown in horde/form's UPGRADING.md.
Blocker: getTypeName() breaks Ingo_Ui_VarRenderer_Html
Ingo still renders through the legacy Horde_Form_Renderer with varrenderer_driver => ['ingo', 'html'] (e.g. spam filtering in lib/Basic/Spam.php). Dispatch is via Horde_Core_Ui_VarRenderer::render(), which builds _renderVarInput_{getTypeName()}.
Ingo_Ui_VarRenderer_Html only implements the legacy names:
_renderVarInput_ingo_form_type_folders→ folder select viaIngo_Flist::select()_renderVarInput_ingo_form_type_longemail→ delegates to_renderVarInput_longtext()
BaseVariable::getTypeName() derives the type from the class namespace:
| Class namespace | getTypeName() |
Renderer method found? |
|---|---|---|
Ingo\Form\V3\FoldersVariable |
ingo_form_type_folders |
Yes |
Horde\Ingo\Form\V3\FoldersVariable |
folders |
No → "Unknown variable type" |
Ingo\Form\V3\LongemailVariable |
ingo_form_type_longemail |
Yes |
Horde\Ingo\Form\V3\LongemailVariable |
longemail |
No → "Unknown variable type" |
After this PR, Forward/Vacation/Spam forms would show warnings instead of folder selectors and email textareas. CI won't catch this — ingo has no form/UI tests for these variables.
Suggested fix (pick one):
- Override
getTypeName()in both Variable classes (matches theBaseVariabledocblock: "Override with a simple return 'literal' string"):
public function getTypeName(): string
{
return 'ingo_form_type_folders'; // or 'ingo_form_type_longemail'
}- Update
Ingo_Ui_VarRenderer_Htmlto add_renderVarInput_folders/_renderVarInput_longemail(and keep the old methods for BC).
Option 1 is smaller and keeps the legacy renderer contract explicit.
A broader fix in horde/form's getTypeName() for Horde\{App}\Form\V3\… would help all apps, but ingo still needs a local fix for merge.
Minor nits
- Empty PR description — worth documenting motivation,
horde/form ^3.1requirement, and manual test plan (spam folder picker, forward/vacation address fields) - Commit typo — second commit says "FCQN"; should be "FQCN"
- Tests — no new coverage; acceptable given ingo's test suite, but manual UI verification is important here
Manual test plan (for author or merger)
- Spam rule page: folder dropdown renders and saves
- Forward rule: multi-line address field renders and validates
- Vacation rule: "My email addresses" and "Addresses to not send responses to" fields render
- With
horde/form3.1.x installed (not onlydev-FRAMEWORK_6_0)
Bottom line
Approve the modernization approach, but do not merge until the getTypeName() / VarRenderer mismatch is fixed. Without that, the namespace change is a user-visible regression on three core ingo screens.
No description provided.